Add clang-tidy static analysis for the NIF - #208
Merged
Conversation
Wire clang-tidy (including the clang static analyzer, via its clang-analyzer-* checks) over the C++ NIF sources in c_src/. Unlike cppcheck, clang-tidy compiles each translation unit, so it needs the MLX/Fine/ERTS headers and the exact build flags. Add a `clang-tidy` Makefile target that reuses the NIF's $(CXXFLAGS), and a `mix clang.tidy` task that supplies the same env elixir_make uses (reusing the cached MLX) before invoking it — mirroring the existing `mix bench.native` pattern. The make target refuses to run without that env rather than emit a confusing empty-`-isystem` clang error. Enabled checks and the header filter (diagnostics scoped to c_src/, never the -isystem MLX/Fine headers) live in a repo-root .clang-tidy; start with a focused bugprone/analyzer/performance set. Add a macOS CI job that installs LLVM via brew and runs `mix clang.tidy`, reusing the precommit lane's MLX cache via restore-keys. Document it in MAINTAINING.md.
Disable three checks that fire systemically on framework-dictated or
deliberate patterns, documented in .clang-tidy:
- performance-unnecessary-value-param: every NIF takes its ResourcePtr
args by value per Fine's FINE_NIF decode convention (the cppcheck
build suppresses the same thing as passedByValueCallback).
- bugprone-throwing-static-initialization: FINE_NIF/FINE_RESOURCE
register at static-init via throwing ctors, across macro expansions.
- performance-enum-size: Opcode and ref::Kind are int64_t on purpose to
pack into the int64 refs the Elixir lowerer emits.
Fix the genuine findings:
- Rename the reserved-identifier namespaces __async/__op (leading double
underscore is reserved) to async_detail/op_detail.
- Bind window-reduce shapes by const reference instead of copying
(performance-unnecessary-copy-initialization), matching the existing
style a few lines up.
- Widen `rank` before the `2 * rank` reserve() so the multiplication
happens in size_t (bugprone-implicit-widening-of-multiplication-result).
Suppress the remaining false positives inline, with rationale:
- The make_binary_from_cstr memcpy targets a length-counted BEAM binary,
not a C string (bugprone-not-null-terminated-result).
- ~Program/~Reaper/~WorkerThread are best-effort cleanup destructors
where a throw is already fatal (bugprone-exception-escape); the check
stays enabled for every other function.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Wire clang-tidy — including the clang static analyzer, via its
clang-analyzer-*checks — over the C++ NIF sources inc_src/, followingthe same shape as the cppcheck integration (make target + macOS CI job + docs).
The one real difference from cppcheck: clang-tidy compiles each translation
unit, so it needs the MLX/Fine/ERTS headers and the exact build flags rather
than running on a bare checkout.
What's here
.clang-tidy(repo root) — a focused, high-signal set (bugprone-*,clang-analyzer-*,performance-*),WarningsAsErrors: '*'to gate, andHeaderFilterRegex: c_src/so diagnostics stay on our own code and never the-isystemMLX/Fine headers. Broaden the set once this baseline is settled.clang-tidyMakefile target — reuses the NIF's exact$(CXXFLAGS)viathe trailing
--. Refuses to run without the build env (pointing atmix clang.tidy) instead of emitting a confusing empty--isystemerror.mix clang.tidytask — the developer entry point. Mirrors the existingmix bench.native: pulls the project'smake_env, addsERTS_INCLUDE_DIR,reuses the already-built/cached MLX, and drives the make target.
brew install llvm+mix clang.tidy, reusing theprecommit lane's MLX cache via
restore-keys(only headers are needed andthe MLX version is unchanged, so no extra source build in the common case).
First-run triage
No
clang-analyzer-*findings (no latent bugs). The rest split into:documented in
.clang-tidy:performance-unnecessary-value-param(Fine'sby-value
FINE_NIFdecode convention — the cppcheck equivalent ofpassedByValueCallback),bugprone-throwing-static-initialization(
FINE_NIF/FINE_RESOURCEregister at static-init), andperformance-enum-size(Opcode/ref::Kindareint64_tfor the packed-refABI).
__async/__op→async_detail/op_detail; bound two window-reduce shapesby
const&instead of copying; widenedrankbefore2 * rankreserves.BEAM-binary
memcpy(not a C string), and the three best-effort cleanupdestructors where a throw is already fatal (the check stays on elsewhere).